Update fee and dust handling for zero fee channels #3884

carlaKC · 2025-06-23T19:48:29Z

This PR completes the off-chain handling of V3 channels, as described in #3789.

ldk-reviews-bot · 2025-06-23T19:48:31Z

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

carlaKC · 2025-06-23T19:48:54Z

This still needs a few tests, opening up early for conceptual review.

carlaKC · 2025-06-23T19:50:39Z

lightning/src/ln/channel.rs

@@ -5078,6 +5092,7 @@ where
 	) -> u64 {
 		if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
 			debug_assert_eq!(self.feerate_per_kw, 0);
+			debug_assert!(fee_spike_buffer_htlc.is_none());


Wondering whether it's worth having this assertion (which makes it necessary to check channel type before calling fee functions). The alternative would be to just ignore the parameters completely for zero fee channels (even though Some fee_spike_buffer_htlc doesn't make sense for the type).

Can't say I have a strong preference. The assertion seems fine to me.

TheBlueMatt

All LGTM, didn't carefully check every hunk, though.

TheBlueMatt · 2025-06-30T21:54:34Z

lightning/src/ln/channel.rs

@@ -133,6 +134,22 @@ enum FeeUpdateState {
 	Outbound,
 }

+/// Returns the fees for success and timeout second stage HTLC transactions.


nit: Probably belongs in chan_utils.rs given it kinda mirrors commit_and_htlc_tx_fees_sat and is also called from there?

TheBlueMatt · 2025-06-30T21:57:54Z

lightning/src/ln/channel.rs

@@ -5078,6 +5092,7 @@ where
 	) -> u64 {
 		if funding.get_channel_type().supports_anchor_zero_fee_commitments() {
 			debug_assert_eq!(self.feerate_per_kw, 0);
+			debug_assert!(fee_spike_buffer_htlc.is_none());


Can't say I have a strong preference. The assertion seems fine to me.

lightning/src/ln/channel.rs

This fee rate is currently used in two scenarios: - To count any fees above what we consider to be a sane estimate towards our dust exposure. - To get a maximum dust exposure (when using MaxDustHTLCExposure::FeeEstimator strategy). When we have zero fee commitments: - Commitments are zero fee, so we don't need to count fees towards dust exposure. - The amount of dust we have is not dependent on fees, as everything is zero fee. - We still want to limit our total dust exposure. This commit updates get_dust_exposure_limiting_feerate to allow a None value to prepare for support for zero fee commitments. This clearly allows us to indicate when we don't care about fee rates for dust considerations. In get_max_dust_htlc_exposure_msat, we simply hardcode a value of 1 sat/vbyte if a feerate dependent strategy is being used.

Co-authored-by: Matt Corallo <[email protected]>

LDK does not support a channel type that supports both zero fee and nonzero fee anchors (as this is impossible), so we can drop the unnecessary check in build_htlc_output.

This commit pulls calculation of second stage fees into a helper function. A side effect of this refactor is that it fixes a rounding issue in commit_and_htlc_tx_fees_sat. Previously, rounding down of feerate would happen after multiplying by the number of HTLCs. Now the feerate will be rounded down before multiplying by the number of HTLCs. This wasn't a serious issue - it would just cause us very slightly over estimate our dust exposure at certain feerates that needed rounding. A hard-coded value in test_nondust_htlc_excess_fees_are_dust is updated to account for this rounding change.

Update second_stage_tx_fees_sat to return zero for zero fee commitments. As is the case for anchors_zero_fee_commitments, this changes ensures that we won't trim the second stage transaction fee off of the HTLC amount.

When we have zero fee commitments, we don't need to calculate our fee rate or check that it isn't stale because it is always zero. Co-authored-by: Matt Corallo <[email protected]>

Co-authored-by: Matt Corallo <[email protected]>

Update test helper in preparation to test zero fee commitments, where the local balance will differ from the previously hardcoded value.

lightning/src/ln/update_fee_tests.rs

ldk-reviews-bot · 2025-07-14T00:01:04Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/ln/channel.rs

tankyleo · 2025-07-14T07:39:54Z

lightning/src/ln/channel.rs

@@ -4775,7 +4779,8 @@ where
 		let excess_feerate_opt = outbound_feerate_update


Is it worth debug asserting that this is always 0 for zero-fee commitments ?

tankyleo · 2025-07-14T07:42:42Z

lightning/src/ln/chan_utils.rs

 #[rustfmt::skip]
 pub(crate) fn htlc_tx_fees_sat(feerate_per_kw: u32, num_accepted_htlcs: usize, num_offered_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
-	let htlc_tx_fees_sat = if !channel_type_features.supports_anchors_zero_fee_htlc_tx() {
-		num_accepted_htlcs as u64 * htlc_success_tx_weight(channel_type_features) * feerate_per_kw as u64 / 1000


In the commit message, I wouldn't say we round a feerate, but rather round a raw transaction fee amount.

tankyleo · 2025-07-14T07:44:07Z

lightning/src/ln/channel.rs

+		let (htlc_success_tx_fee_sat, htlc_timeout_tx_fee_sat) =
+			second_stage_tx_fees_sat(features, feerate_per_kw);
+
+		let htlc_tx_fee_sat = if local {


nit: keep the formatting consistent between the two is_dust functions ?

lightning/src/ln/channel.rs

tankyleo · 2025-07-14T07:59:47Z

lightning/src/ln/update_fee_tests.rs

+	// Sender should not queue an update_fee message.
+	nodes[0].node.timer_tick_occurred();
+	let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events_0.len(), 0);


I wouldn't expect an update_fee to be generated in any channel type here.

This test has this log here:

node 0 TRACE [lightning::ln::channelmanager:6975] Channel 5109930e509248b4fdac5f8fef66915f693393cdbfd8f0cd2d56f0f4d80baf62 qualifies for a feerate change from 0 to 0.

Looks like we should update the condition to skip a fee update in fn update_channel_fee:

if new_feerate <= chan.context.get_feerate_sat_per_1000_weight() && new_feerate * 2 > chan.context.get_feerate_sat_per_1000_weight() {

Looks like we should update the condition to skip a fee update in fn update_channel_fee:

I don't really feel like it's channel manger's responsibility to know that we shouldn't be sending fee updates for zero fee commitments (belongs in channel IMO), but checking "has this fee changed" is fair game so will look at updating 👍

Right seems the condition is true for all cases where the old feerate is equal to the new feerate, except when that feerate is 0 :)

tankyleo · 2025-07-14T08:04:14Z

lightning/src/ln/channel.rs

-			// side, only on the sender's. Note that with anchor outputs we are no longer as
-			// sensitive to fee spikes, so we need to account for them.
+			// `Some(())` is for the fee spike buffer we keep for the remote if the channel is
+			// not zero fee.. This deviates from the spec because the fee spike buffer requirement


nit: delete the two dots

elnosh

Still grokking some of the changes here and the zero-fee commitment proposal but from my kind of limited context it LGTM (:

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

carlaKC commented Jun 23, 2025

View reviewed changes

carlaKC mentioned this pull request Jun 23, 2025

[Custom Transactions] Add TxBuilder trait, support fixed additional outputs #3775

Merged

carlaKC added this to Weekly Goals Jun 26, 2025

carlaKC moved this to Goal: Open in Weekly Goals Jun 26, 2025

TheBlueMatt reviewed Jun 30, 2025

View reviewed changes

jkczyz assigned carlaKC Jul 3, 2025

carlaKC force-pushed the 3789-fees-and-dust branch from 001c8e9 to 4157d24 Compare July 10, 2025 13:16

carlaKC marked this pull request as ready for review July 10, 2025 13:32

ldk-reviews-bot requested a review from joostjager July 10, 2025 13:32

carlaKC removed the request for review from joostjager July 10, 2025 13:32

graphite-app bot reviewed Jul 10, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

carlaKC and others added 10 commits July 10, 2025 09:39

ln/refactor: add utility to get commitment feerate by channel type

1b70e6d

ln: add channel type awareness to get_dust_exposure_limiting_feerate

5329947

Co-authored-by: Matt Corallo <[email protected]>

ln: remove unnecessary nonzero anchor channel check in build_htlc_output

dbffbcb

LDK does not support a channel type that supports both zero fee and nonzero fee anchors (as this is impossible), so we can drop the unnecessary check in build_htlc_output.

ln: set zero second stage fees for zero fee commitments

0af83b8

Update second_stage_tx_fees_sat to return zero for zero fee commitments. As is the case for anchors_zero_fee_commitments, this changes ensures that we won't trim the second stage transaction fee off of the HTLC amount.

ln: return early and assert in fee calculation and checks

507fb6d

When we have zero fee commitments, we don't need to calculate our fee rate or check that it isn't stale because it is always zero. Co-authored-by: Matt Corallo <[email protected]>

ln: do not send and reject receive of update_fee for zero fee commits

5a92fba

Co-authored-by: Matt Corallo <[email protected]>

ln: add test helper for fee spike buffer test and use TxBuilder

01efb46

Update test helper in preparation to test zero fee commitments, where the local balance will differ from the previously hardcoded value.

ln: do not maintain fee spike buffer for zero fee commitment channels

5fe6fa7

carlaKC force-pushed the 3789-fees-and-dust branch from 4157d24 to 5fe6fa7 Compare July 10, 2025 13:40

graphite-app bot reviewed Jul 10, 2025

View reviewed changes

lightning/src/ln/update_fee_tests.rs Show resolved Hide resolved

carlaKC mentioned this pull request Jul 10, 2025

[Custom Transactions] Abstract the weight of HTLC transactions away from channel #3921

Draft

tankyleo self-requested a review July 11, 2025 16:49

tankyleo reviewed Jul 14, 2025

View reviewed changes

elnosh reviewed Jul 14, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

		@@ -4775,7 +4779,8 @@ where
		let excess_feerate_opt = outbound_feerate_update

Update fee and dust handling for zero fee channels #3884

Are you sure you want to change the base?

Update fee and dust handling for zero fee channels #3884

Conversation

carlaKC commented Jun 23, 2025

Uh oh!

ldk-reviews-bot commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlaKC commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 14, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elnosh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 23, 2025 •

edited

Loading